r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

cc @est31 @ehuss

cc @Nadrieril

This needs a fcp so I'd like to roll this to someone more familiar with this feature
r? compiler

r? @est31

The reference PR was (before this r-) marked S-waiting-on-stabilization so I would have assumed that means that this was not blocked on the reference PR and that it was approved

Yes, we on lang-docs could probably use to better document the system here.

In this case, the intended signals were that:

• It was still marked S-waiting-on-review.
• It lacks an approving review.
• This issue is marked S-waiting-on-documentation.

(That the Reference PR was also marked S-waiting-on-stabilization was additive, and is our signal that even after we remove S-waiting-on-review, we still should not merge it until the stabilization is finalized. Sometimes, even then, we hesitate to leave an approving review if the stabilization has not yet finished FCP, as sometimes on the lang side, we change things about the stabilization, and so on the Reference side, we like to take a last look before approving to be sure the Reference PR is still correct at the end of the FCP.)

☔ The latest upstream changes (presumably #142220) made this pull request unmergeable. Please resolve the merge conflicts.

I'm sorry to bring this up after the FCP, but having worked some more on implementing guard patterns, I've noticed the drop order here actually isn't quite what I'd expect, and there are in fact still some missing drop order tests. All the tests I can find in #140981 for if let guards are for the temporary drop order among guards in let chains; that order does look good. But I can't find any tests comparing the drop order of if let guard temporaries to the pattern bindings for the match arm the guard is on. Here's an example (playground):

assert_drop_order(1..=3, |o| {
    // `if let` guards' drops are *after* pattern bindings'
    match [LogDrop(o, 2), LogDrop(o, 1)] {
        [x, y] if let z = LogDrop(o, 3) => {}
        _ => unreachable!(),
    }
});
    
assert_drop_order(1..=3, |o| {
    // I would expect this drop order.
    match [LogDrop(o, 3), LogDrop(o, 2)] {
        [x, y] => {
            let z = LogDrop(o, 1);
        }
    }
});

This also means dropck problems (playground):

// `x` is dropped first, resulting in a dropck failure.
match SignificantDrop(0) {
    x if let y = SignificantDrop(&x) => {}
    _ => unreachable!(),
}
// `y` is dropped first; everything's fine.
match SignificantDrop(0) {
    x => {
        let y = SignificantDrop(&x);
    }
}

y still wouldn't be usable in the match arm in that first example even if everything was fixed, due to the binding of x moving out of the scrutinee (iiuc, that first y = SignificantDrop(&x) is relative to the scrutinee, not the binding of x used in the arm). But y should still be usable later in the guard. Binding x by reference would normally also lead to a dropck failure in this case (while still leaving y usable in the arm), but by-ref bindings are dropped last when there's a guard (#142057), so dropck doesn't complain.

Great catch. That does violate my expectations. Thanks @dianne for bringing that to our attention.

@rfcbot concern drop-order-bug-found-by-dianne

@rustbot author

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Given the near miss -- despite the care we took to avoid it -- it now feels like, for this and these sort of things, maybe we should expect proof that the testing is exhaustive against the code that's expected to be equivalent, e.g. by some automated means to generate tests for all possible cases.

@dianne, this is a valuable catch, there is nothing to sorry about, it's better to find this before we stabilize the feature, rather than after

about your test, the reason i was confident about this part because we had this test compare-if-let-guard which shows that there is not differences between if let inside match's arm and if let guard, but, it doesnt show exactly the problem with dropping match bindings (which i was not knowing about)

i analyzed the MIR and can confirm the problem

    match Some(1) {
        Some(_x) if let Some(_y) = Some(2) => {},
        _ => {}
    }

for some reasons compiler creates guard bindings before pattern bindings

    bb2: {
        _5 = copy ((_6 as Some).0: i32); // _6 = Option::<i32>::Some(const 2_i32);
        _3 = copy ((_1 as Some).0: i32); // _1 = Option::<i32>::Some(const 1_i32);
        goto -> bb3;
    }

this means pattern bindings would be dropped first (reverse order), causing the dropck issues you identified

I believe creating guard bindings before pattern bindings is intentional: it's needed for https://doc.rust-lang.org/reference/expressions/match-expr.html#r-expr.match.guard.value. I think the drops will need manual rescheduling to untwist them¹; some amount of manual rescheduling is already done for or-patterns, so there's precedent. Ideally I'd like to implement this by specifying the drop order explicitly, but that's soft-blocked on #142163² (and it might just be easier anyway to use the drop order we get from lowering the guard, but shuffled to happen after the arm's bindings are dropped).

I'm not sure if the team has already seen #142163, @traviscross, but I wanted to bring it to your attention. It's a fairly significant issue, and if it wasn't brought up at the last meeting a couple of days ago, I think it would be worth putting on the agenda soon (though I'm not entirely familiar with how these meetings are planned)

As for the issue itself, I currently see two possible approaches:

1. Document the current behavior and treat it as non-blocking — as far as I understand, this behavior isn’t exclusive to if let guards but is part of a more general issue. Also, from what I can tell, it's not something that can be used to break soundness — for example, the borrow checker already prevents misuses in the if let context. If we fix this issue after stabilization, as I understand it, this probably won’t break backwards compatibility (please correct me or share your opinion on this point, @dianne)

2. Wait until the issue is resolved, and proceed with stabilization only after that

I’m not saying this isn’t a real issue — it definitely is — but it seems to be more of an external problem rather than one specific to this feature. I'm not sure what the best path forward for this stabilization PR is, so I just wanted to share the two directions which are the most obvious to me

as far as I understand, this behavior isn’t exclusive to if let guards but is part of a more general issue.

That's also my understanding. The only thing that makes if let guards special is that their scrutinee/bindings are created/declared in the match arm's scope. Thus, explicitly specifying the drop order for a match arm as a whole requires explicitly specifying that drop order (which makes it less convenient). That's not the only way to resolve the issue with let guards, though, so I don't think it would need to block let guard stabilization. I think the easiest solution to let guard drop order currently would be to use the drop order we cget from lowering guards, but move them to happen before the arm's pattern's drops, keeping the ordering the same as it is now besides that. This wouldn't require solving the or-patterns issue.

There might also be reason to specify and stabilize let guards, as-is, with guards' bindings and scrutinees being dropped after the arm's pattern's. I've been trying to think in terms of what's needed for guard patterns to find reasons to keep/untwist the drop order, but so far I haven't come up with anything in those terms beyond "implementation convenience". Even for hypothetical if let guard patterns, all I've found so far is that they're more complicated than I previously had thought ^^;

If we fix this issue after stabilization, as I understand it, this probably won’t break backwards compatibility

Fixing #142163 is a breaking change regardless, but I'd consider it mostly orthogonal to let guards. let guards do increase its surface area in a way, by adding more ways to write patterns, but in many cases they'd be written as patterns regardless of whether the language has let guards. I have been in a situation where let guards would have let me write things as patterns that I otherwise wouldn't have been able to, but I also didn't depend on drop order or or-patterns. I'm not sure it's possible to get a perfect picture of how the impact of fixing #142163 changes based on stabilizing let guards, but I think it's minimal. I just happened to find a lot of drop order inconsistencies in a short timeframe because they affect the implementation of guard patterns.

I'm curious to hear from @est31 here as well. Even if it's part of a more general issue, it'd be better if possible if we could do something here to avoid increasing the surface area of it, and I'm curious to hear from everyone what the options might be there.

Separately, I'm curious to hear if there might be some structured way that we could test this to assure ourselves that we really have covered all of the possible drop order cases before the next attempt at stabilization.

☔ The latest upstream changes (presumably #137944) made this pull request unmergeable. Please resolve the merge conflicts.

Thanks @dianne for the great catch. I'd say it's definitely not the behaviour I expected, and we should change it, and that's a blocker for stabilization. I wouldn't put all of #142163 into the path though, that should be its own issue.

Drop order has a bunch of weird edge cases, like the thing where panicking in an array or vec constructor reverses the drop order of the array's items, and it's guaranteed to be stable via RFC 1857. Maybe we can change some weirdnesses pointed out in #142163 as well. The let chains stabilization has caused general weirdness fixes #103293 and #103108, but those were in the blocker path for stabilization.

In #140981, I have tried to use the edge cases known from if let chains to test if let guards. Those work indeed really well, but I suppose with all that attention on chains I missed the more basic, non-chain drop order issue found by @dianne. I guess it slipped through because if let chains don't have that extra scrutinee.

I'm curious to hear if there might be some structured way that we could test this to assure ourselves that we really have covered all of the possible drop order cases

The surface area is huge. There have been attempts at doing exhaustive drop order checks, like #100526, #99291, and most recently #133605. RFC 1857's title is very generic but it only considers a small slice of the surface area as well (structs, vecs, etc). One issue of exhaustive checks is that they only consider the current language, and not all of its possible extensions.

That doesn't mean we can't try to make a test for if let chains as exhaustive as possible. We just need to go through all the places that potentially create temporaries, and add ManuallyDrop(_, n) around them.

The relevant piece here wasn't that we didn't have a drop order test with a scrutinee and a guard, it was that the existing tests didn't make the scrutinee temporary ManuallyDrop.

There is still unexplored territory though, i.e. do we drop before or after code execution reaches the next arm's if let (we probably drop after, and we should probably drop before).

There is still unexplored territory though, i.e. do we drop before or after code execution reaches the next arm's if let (we probably drop after, and we should probably drop before).

Thankfully, let guard temporaries are already dropped before the next arm, though we're likely missing a test for it. As I understand it, their temporary scope (and the scope of their bindings) is the scope of the arm they're on, so they're dropped when leaving the arm, whether that's through getting to the end of the arm's body, from a guard failing, or from breaking out early. This is also consistent between match arms and or-patterns: conceptually, failing a guard leaves the arm's scope, then trying another or-pattern alternative re-enters it¹. Here's an example (playground):

assert_drop_order(1..=6, |o| {
    let mut x = 1;
    // The match's scrutinee is kept until the end of the match.
    match o.log(6) {
        // Failing a guard breaks out of the arm's scope, dropping
        // the `let` guard's scrutinee. To test the guard for the
        // next or-pattern alternative, we re-enter the arm's scope.
        _ | _ | _ if let _ = o.log(x)
            && { x += 1; false } => unreachable!(),
        // If the guard succeeds, we stay in the arm's scope so the
        // `let` guard's scrutinee is kept around. As before, we
        // drop it when leaving the arm's scope.
        _ if let _ = o.log(5)
            && true => { o.log(4); }
        _ => unreachable!(),
    }
});